Firestore: settings.ts: very minor refactor of long-polling logic. #7239
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Report 1Affected Products
Test Logs |
| if (!settings.experimentalForceLongPolling) { | ||
| this.experimentalForceLongPolling = false; | ||
| this.experimentalAutoDetectLongPolling = | ||
| settings.experimentalAutoDetectLongPolling ?? false; |
There was a problem hiding this comment.
I think we should respect previous code that would ensure runtime type is boolean.
I prefer using Boolean function instead of !!, but your call.
This was helpful to me when considering trade off: https://gist.github.com/arthurvi/66cb1e2bcfc92f99f465e0db04264367
ALSO: The other PR seems to have logic reversed settings.experimentalAutoDetectLongPolling ?? true. Is this intentional?
There was a problem hiding this comment.
Good point about coercing to boolean. I've changed the code to first check if settings.experimentalAutoDetectLongPolling === undefined, after which TypeScript narrows the type of settings.experimentalAutoDetectLongPolling to boolean (from boolean|undefined).
Technically, it is still possible that some non-boolean value is specified but we don't tend to do type checking in our code base for things that the TypeScript compiler would detect at compile time. If you feel strongly that I should still coerce the type to boolean then I will; however, note that this causes a warning in IntelliJ:
There was a problem hiding this comment.
The other PR seems to have logic reversed
Yes, that is intentional. In fact, the entire purpose of the follow up PR #7236 is to change the default to true from false.
There was a problem hiding this comment.
As a tangent, we tend to use !! in our code base, for better or worse. I can't think of any time I've seen Boolean() used. So, when choosing between the two I'd pick the one that matches the local convention, namely !!.
There was a problem hiding this comment.
TLDR: All this will be very opinionated, so in the end, I am ok with original code and your new code, so I will simply approve. Feel free to make further changes if you want.
Here is the TL part:
Looking at surrounding code and digging a little deeper, I do see many things worth noting.
- IntelliJ doesn't complain when using
Boolean(..)instead of!!. DEFAULT_SSLuses the??operator, so if it is a problem here, it will also be a problem there.- Handling these corner cases is increasing code size, but adding questionable value. I think there is a trade off to consider.
DEFAULT_SSLintroduces a constant. That pattern, if followed, implies that long polling should also have constants. But this only serves to bloat the code further.- I am not sure whether minification will help with any of the above. I suspect not.
There was a problem hiding this comment.
Thanks for these thoughts, Tom.
I've opened a new PR, #7242, which expands the unit test suite to also verify how "truthy" and "falsy" values are coerced into boolean. PTAL if you don't mind. I wanted to add those tests in a separate PR (rather than including them in this PR) in order to "prove" that there are no behavior changes introduced by this PR.
To avoid unintentionally introducing any backwards incompatibilities, I'm also going to modify this PR to coerce to boolean. I've also modified the logic to more closely mirror that of DEFAULT_SSL, as you pointed out. Namely, I've added a new constant, DEFAULT_AUTO_DETECT_LONG_POLLING, and used the ?? operator where appropriate.
Personally, I'm not concerned about the code size increases from changes like this since the delta will be minuscule.
Size Analysis Report 1Affected Products
Test Logs |
…LongPolling is boolean
tom-andersen
approved these changes
Apr 20, 2023
…LongPolling and experimentalAutoDetectLongPolling to boolean
…lines to make lint happy
dconeybe
changed the base branch from
master
to
dconeybe/LongPollingCoerceToBooleanTests
Base automatically changed from
dconeybe/LongPollingCoerceToBooleanTests
to
master
April 20, 2023 20:34
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This minor refactor makes a follow-up PR, #7236, that enables long-polling by default more focused. Otherwise, no logic is changed by this PR; only the code is reorganized a bit.